Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor issue filter (labels, poster, assignee) #32771

Merged
merged 6 commits into from
Dec 10, 2024

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Dec 9, 2024

Rewrite a lot of legacy strange code, remove duplicate code, remove jquery, and make these filters reusable.

Let's forget the old code, new code affects:

  • issue list open/close switch
  • issue list filter (label, author, assignee)
  • milestone list open/close switch
  • milestone issue list filter (label, author, assignee)
  • project view (label, assignee)

Next plan: add the user filter and label filter to other pages, eg: user/org home issue list

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 9, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 9, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/frontend labels Dec 9, 2024
@wxiaoguang wxiaoguang force-pushed the refactor-issue-filter branch from a3bcc28 to 7afa821 Compare December 9, 2024 13:16
@wxiaoguang wxiaoguang force-pushed the refactor-issue-filter branch from 7afa821 to 0dcb4a1 Compare December 9, 2024 14:14
@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 9, 2024
@wxiaoguang wxiaoguang force-pushed the refactor-issue-filter branch 4 times, most recently from 60f66c5 to 8e961fa Compare December 9, 2024 15:22
@wxiaoguang wxiaoguang changed the title Refactor issue filter (labels, poster) Refactor issue filter (labels, poster, assignee) Dec 9, 2024
@wxiaoguang wxiaoguang force-pushed the refactor-issue-filter branch from 8e961fa to f84d018 Compare December 9, 2024 15:23
@wxiaoguang
Copy link
Contributor Author

Major work is done, ready for review

@wxiaoguang wxiaoguang added this to the 1.23.0 milestone Dec 9, 2024
@wxiaoguang wxiaoguang added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Dec 9, 2024
@wxiaoguang wxiaoguang force-pushed the refactor-issue-filter branch from 60efb57 to 1dbd446 Compare December 9, 2024 16:10
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 10, 2024
var s string
if len(a)%2 == 1 {
if v, ok := a[0].(string); ok {
if v == "" || (v[0] != '?' && v[0] != '&') {
panic("queryBuild: invalid argument")
panic("QueryBuild: invalid argument")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to panic? Is it possible to recover from a panic if necessary?

Copy link
Contributor Author

@wxiaoguang wxiaoguang Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to panic?

Yes. Golang also does so.

Is it possible to recover from a panic if necessary?

No

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a user's perspective, no one wants to cause the entire system to panic because of a query. I think this is unreasonable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a user's perspective, no one wants to cause the entire system to panic because of a query. I think this is unreasonable.

There are a variety of operations in Golang that may cause a panic. For this case, I think it is the developer's responsibility to check and ensure that the query does not cause a panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a user's perspective, no one wants to cause the entire system to panic because of a query. I think this is unreasonable.

No, it won't panic from "user's perspective". It only panics for developer's mistakes.

It is designed carefully and intentionally, I won't change it unless you could show a real case why end users would be affected.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 10, 2024
@wxiaoguang wxiaoguang merged commit 90d20be into go-gitea:main Dec 10, 2024
26 checks passed
@wxiaoguang wxiaoguang deleted the refactor-issue-filter branch December 10, 2024 03:38
silverwind added a commit to silverwind/gitea that referenced this pull request Dec 10, 2024
* origin/main:
  Fix internal server error when updating labels without write permission (go-gitea#32776)
  Fix wiki ui (go-gitea#32781)
  Change typescript `module` to `nodenext` (go-gitea#32757)
  Refactor issue filter (labels, poster, assignee) (go-gitea#32771)
  Make RepoActionView.vue support `##[group]` (go-gitea#32770)
  [skip ci] Updated translations via Crowdin
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 11, 2024
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Use batch database operations instead of one by one to optimze api pulls (go-gitea#32680)
  Fix internal server error when updating labels without write permission (go-gitea#32776)
  Fix wiki ui (go-gitea#32781)
  Change typescript `module` to `nodenext` (go-gitea#32757)
  Refactor issue filter (labels, poster, assignee) (go-gitea#32771)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/frontend modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/translation size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants